-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pubsub: fix permadiff with configuring an empty retry_policy. #11834
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 53 Click here to see the affected service packages
Action takenFound 26 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
@@ -471,6 +471,7 @@ properties: | |||
A duration in seconds with up to nine fractional digits, terminated by 's'. Example: "3.5s". | |||
default_from_api: true | |||
diff_suppress_func: 'tpgresource.DurationDiffSuppress' | |||
send_empty_value: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll want allow_empty_object here as well, though it looks like the tests are passing, so maybe it's fine? Did you happen to try that out?
send_empty_value: true | |
send_empty_value: true | |
allow_empty_object: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the earlier BQ-related test failures seem unrelated and are passing now. Thanks for opening hashicorp/terraform-provider-google#19683.
Those tests pass locally as well:
$ make testacc TEST=./google-beta/services/pubsub TESTARGS='-run=TestAccPubsubSubscription_pubsubSubscriptionPushBqExample'
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta/services/pubsub -v -run=TestAccPubsubSubscription_pubsubSubscriptionPushBqExample -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN TestAccPubsubSubscription_pubsubSubscriptionPushBqExample
=== PAUSE TestAccPubsubSubscription_pubsubSubscriptionPushBqExample
=== CONT TestAccPubsubSubscription_pubsubSubscriptionPushBqExample
--- PASS: TestAccPubsubSubscription_pubsubSubscriptionPushBqExample (30.70s)
PASS
ok github.com/hashicorp/terraform-provider-google-beta/google-beta/services/pubsub 31.317s
$ make testacc TEST=./google-beta/services/pubsub TESTARGS='-run=TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample'
==> Checking that code complies with gofmt requirements...
go vet
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta/services/pubsub -v -run=TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample
=== PAUSE TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample
=== CONT TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample
--- PASS: TestAccPubsubSubscription_pubsubSubscriptionPushBqTableSchemaExample (30.34s)
PASS
ok github.com/hashicorp/terraform-provider-google-beta/google-beta/services/pubsub 30.958s
Similarly, the "VCR-test" was flaky as well but is passing now. However, the terraform-google-conversion-build-and-unit-tests
failures show a permadiff with "retryPolicy": nil,
(example test case and test code throwing the error). I think that I'd need to submit a separate PR to fix those tests since they are in a different repo.
I think you'll want allow_empty_object here as well, though it looks like the tests are passing, so maybe it's fine? Did you happen to try that out?
I didn't see that in the field reference but I see it here and used elsewhere in Subscription.yaml
. But yeah, the tests are passing with or without allow_empty_object
and I'm not sure why, but it doesn't seem like adding allow_empty_object
will hurt so I've added it.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 55 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 55 Click here to see the affected service packages
🟢 All tests passed! View the build log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like send_empty_value
means that the nil value also needs to be included in TGC tests. Basically that means adding it to the data for both suscriptions in https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/tgc/tests/data/example_pubsub_subscription.json, and also adding it to the subscription data in the example_pubsub_subscription_iam_*.json files in the same directory.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 55 Click here to see the affected service packages
🟢 All tests passed! View the build log |
Ah right, thanks. Updated and all tests are passing. I'm not 100% clear on why we needed to change these tests but not the existing tests here however. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% clear on why we needed to change these tests but not the existing tests here however.
send_empty_value
is only supposed to send empty values to the API if they are explicitly set in the user config but they were not in the affected TGC tests.
It looks like the send_empty_value docs are incorrect - the resource will also send the value if the field isn't set. This is probably why all the tests had to be re-recorded here: #11834 (comment) (because they all started sending the field as null
, which didn't match the previous requests.)
As long as that's the same for the API as not sending the field, this should be fine. Let me know!
Yeah it shouldn't be an issue for the API (as evidenced by the existing acceptance tests passing). I was just concerned about potentially breaking folks' Terraform code, but I don't think that will be a problem either. |
Fixes hashicorp/terraform-provider-google#18016
The permadiff arises when users specify an empty retry policy as
retry_policy: {}
in their config, but the empty value doesn't get sent to the API and the API subsequently doesn't return aretry_policy
in the response. By settingsend_empty_value: true
on theretry_policy
property, we ensure that the API recognizes the user-configured value.Similar fixes for other resource properties will be investigated and sent in separate PRs.
Release Note Template for Downstream PRs (will be copied)